Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

gm_notes: allow custom monsters #164

Merged
merged 5 commits into from
Mar 11, 2024

Conversation

kloetzl
Copy link
Contributor

@kloetzl kloetzl commented Mar 2, 2024

GM Notes were limited to preexisting monsters.

@canismarko
Copy link
Owner

Hi, @kloetzl. Thanks for the contribution.

If I understand this right, the monsters will now need to be classes, right? I don't really have a preference which way works. I wonder if the existing system is compatible with what you're trying to do?

Does it work for your use case if you add this to your gm notes file (e.g. gm_notes.py)? I haven't run this code, so I mean it more as an illustration, modify as needed.

from dungeonsheets import Monster

class MyMonster(Monster):
    ...

monsters = [MyMonster(), ...]

It would then be an instance of monsters.Monster and would pass fine.

If that works, then I think this is actually a documentation bug. I will update the docs and examples to make this more clear.

If not, I would propose the following changes to your PR before merging.

  1. Include an elif so that we can handle both isinstance(monster, Monster) and issubclass(monster, Monster). Otherwise existing gm_notes break.
  2. Add an example of this new usage to examples/gm-session-notes.py

Also, why are you testing that isinstance(monster, type)? Would that automatically be true if issubclass(monster, Monster) is true? Not that I plan this, but adding this check closes the possibility of metaclasses for Monster (maybe we want abstract base classes or something?).

Let me know your thoughts.

@kloetzl
Copy link
Contributor Author

kloetzl commented Mar 4, 2024

Apologies, I got myself a bit confused when writing this PR. For magic items, spells, classes, etc. the user always has to provide the class to the array (or at least that is how it's done in the documentation). It is a bit weird that for monsters it has to be an instance of said class. I assumed that this inconsistency was just an oversight.

I've incorporated your suggestions. Feel free to simplify the example.

@canismarko
Copy link
Owner

I assumed that this inconsistency was just an oversight.

It probably is to be honest. But since it's there, we shouldn't break it.

Looks like there's a missing import in the example, once that's fixed I'm good to merge.

@canismarko
Copy link
Owner

Looks good, thanks.

@canismarko canismarko merged commit db3a05f into canismarko:master Mar 11, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants